Skip to content

Conversation

@fscnick
Copy link
Collaborator

@fscnick fscnick commented Jan 12, 2026

Why are these changes needed?

This PR provides the unit test and e2e test for #4160.

  • it uses synctest for the unit test
  • it applies the feature gate to run the existed e2e test.

Related issue number

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

Comment on lines +107 to +108
// Test with getting a persistent error, the cache should be removed eventually.
nonExistedJobId := "not-existed-job-id"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another test in the same bubble. The goroutine is created by singleton and synctest is not allowed to access goroutine outside of bubble.

WithRayClusterSpec(NewRayClusterSpec(MountConfigMap[rayv1ac.RayClusterSpecApplyConfiguration](jobs, "/home/ray/jobs"))).
WithEntrypoint("python /home/ray/jobs/long_running.py").
WithActiveDeadlineSeconds(45). // Short deadline for failing the JobDeploymentStatus, but making sure the cluster is running
WithActiveDeadlineSeconds(60). // Short deadline for failing the JobDeploymentStatus, but making sure the cluster is running
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a few second to avoid flaky.

Comment on lines 533 to 536
g.Eventually(RayJob(test, rayJob.Namespace, rayJob.Name), TestTimeoutMedium).
Should(And(
WithTransform(RayJobStatus, Equal(rayv1.JobStatusRunning)),
WithTransform(RayJobReason, Equal(rayv1.DeadlineExceeded))))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use Eventually to wait for the condition satisfied.

@fscnick fscnick marked this pull request as ready for review January 13, 2026 11:50
@fscnick fscnick force-pushed the feat/background-goroutine-get-job-info-test branch 2 times, most recently from f8c8afb to 5ea0272 Compare January 14, 2026 00:45
@Future-Outlier
Copy link
Member

cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no bugs!

Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @seanlaii @400Ping to take a look

@Future-Outlier
Copy link
Member

cc @AndySung320 to take a look

Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we only keep testing AsyncJobInfoQuery=false?
since this is not yet in beta version or GA?

Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, cc @rueian to merge
but I personally think we should keep testing AsyncJobInfoQuery=false

#4368 (review)

@fscnick fscnick force-pushed the feat/background-goroutine-get-job-info-test branch from 5ea0272 to 18d6c84 Compare January 14, 2026 14:22
@fscnick
Copy link
Collaborator Author

fscnick commented Jan 14, 2026

Should we only keep testing AsyncJobInfoQuery=false? since this is not yet in beta version or GA?

I'm not that certain about it, kindly let me know if I was wrong.

Isn't AsyncJobInfoQuery=false for the nightly build ? Add it to the test for ensuring the new feature doesn't break the existed test.

else
IMG=kuberay/operator:nightly make docker-image &&
kind load docker-image kuberay/operator:nightly &&
echo "Deploying operator with test overrides (feature gates via test-overrides overlay)"
IMG=kuberay/operator:nightly make deploy-with-override
fi

@rueian
Copy link
Collaborator

rueian commented Jan 14, 2026

Isn't AsyncJobInfoQuery=false for the nightly build ? Add it to the test for ensuring the new feature doesn't break the existed test.

Can we test both AsyncJobInfoQuery=false and AsyncJobInfoQuery=true?

# Build nightly KubeRay operator image
- pushd ray-operator
- IMG=kuberay/operator:nightly make docker-image && kind load docker-image kuberay/operator:nightly && echo "Deploying operator with test overrides without AsyncJobInfoQuery (feature gates via test-overrides-without-async-job-info-query overlay )"
- IMG=kuberay/operator:nightly make deploy-with-override-without-async-job-info-query
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an additional e2e test with a different make command to test them with AsyncJobInfoQuery=true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants